Allow atomic operations on non-shared memory in SafeHeap#8890
Conversation
| bool | ||
| isPossibleAtomicOperation(Index align, Index bytes, bool shared, Type type) { | ||
| return align == bytes && shared && type.isInteger(); | ||
| isPossibleAtomicOperation(Index align, Index bytes, bool /*shared*/, Type type) { |
There was a problem hiding this comment.
If any human or bot gets a chance to look at this, please remove the newly unused parameter.
|
Yeah I was discussing this with @brendandahl and we think the first fix to the emscripten breakage that prompted this (see triage_20260708_084511.html on the internal x20 link) would be to ensure that we link with |
|
When libraries using atomic are compiled with atomics enabled, those atomics are not lowered away. Then we allow those libraries to be linked into single-threaded applications, which will not have shared memories. Presumably SafeHeap will want to instrument the resulting atomic memory accesses to unshared memories the same as it would any other memory access, so I think this PR is a correct fix. |
|
True; but IIRC there were some cases where we do lower atomics away after linking (for browsers that don't support atomic at all). I wasn't sure whether this case (where were "incorrectly" failling to use -pthread together with openMP) was supposed to have that happen. Or maybe it would have happened after this particular pass. |
|
Going back to the change itself, this just means that we'll generate atomic helper functions in many cases where we don't today, but that seems fine. I can go ahead and fix this up, write a test, etc. |
Oh interesting, I wasn't aware of that. I agree this change is correct regardless of that, though. |
SafeHeap needs to generate atomic helper functions even if the memory is not shared, because atomic instructions can still be used when threads feature is enabled. Also, removed the now-unused 'shared' parameter from isPossibleAtomicOperation, added a test case, and updated existing tests. TAG=agy CONV=75c799ef-6dca-4712-8ffe-d8d1dfaffd0b
dschuff
force-pushed
the
agy-fix-safeheap-atomics
branch
from
cf9abe2 to
02003f3
Compare
|
OK, I've updated to fix the function and added a test. Several existing tests had threads enabled but unshared memories. they how have atomic helper functions where they didn't before. |
2 participants
SafeHeap needs to generate atomic helper functions even if the memory
is not shared, because atomic instructions can still be used when
threads feature is enabled.
Also, removed the now-unused 'shared' parameter from
isPossibleAtomicOperation, added a test case, and updated existing tests.